Skip to content

zephyr: trace: use zephyr utilities when enabled#4227

Merged
lgirdwood merged 1 commit intothesofproject:mainfrom
iuliana-prodan:trace_warnings
May 21, 2021
Merged

zephyr: trace: use zephyr utilities when enabled#4227
lgirdwood merged 1 commit intothesofproject:mainfrom
iuliana-prodan:trace_warnings

Conversation

@iuliana-prodan
Copy link
Contributor

@iuliana-prodan iuliana-prodan commented May 20, 2021

Use the Zephyr sys/printk.h when Zephyr RTOS is used.

Suggested-by: Guennadi Liakhovetski [email protected]
Signed-off-by: Iuliana Prodan [email protected]

@iuliana-prodan
zephyr: trace: use zephyr utilities when enabled
417c524 
Use the Zephyr sys/printk.h when Zephyr RTOS is used.

Suggested-by: Guennadi Liakhovetski <[email protected]>
Signed-off-by: Iuliana Prodan <[email protected]>
@iuliana-prodan iuliana-prodan requested a review from akloniex as a code owner May 20, 2021 15:47
@iuliana-prodan
Copy link
Contributor Author

iuliana-prodan commented May 20, 2021

This patch fixes the following warnings when building SOF with Zephyr on i.MX:

60/222] Building C object modules/sof/CMakeFiles/modules_sof.dir sof/sof/src/lib/dai.c.obj
In file included from sof/sof/zephyr/../src/include/sof/lib/dai.h:27,
                 from  sof/sof/src/lib/dai.c:7:
sof/sof/src/lib/dai.c: In function 'dai_group_get':
sof/sof/zephyr/include/sof/trace/trace.h:35:4: warning: implicit declaration of function 'printk'; did you mean 'cbprintf'? [-Wimplicit-function-declaration]
   35 |    printk("%llu: " format "\n", platform_timer_get(NULL), \
      |    ^~~~~~
sof/sof/zephyr/include/sof/trace/../../../../src/include/sof/trace/trace.h:287:2: note: in expansion of macro '_log_message'
  287 |  _log_message(trace_log_filtered, true, LOG_LEVEL_CRITICAL, class, ctx, id_1, \
      |  ^~~~~~~~~~~~
sof/sof/zephyr/include/sof/trace/../../../../src/include/sof/trace/trace.h:290:2: note: in expansion of macro '_trace_error_with_ids'
  290 |  _trace_error_with_ids(class, ctx, id_1, id_2, format, ##__VA_ARGS__)
      |  ^~~~~~~~~~~~~~~~~~~~~
sof/sof/zephyr/include/sof/trace/../../../../src/include/sof/trace/trace.h:374:2: note: in expansion of macro 'trace_error_with_ids'
  374 |  trace_error_with_ids(_TRACE_INV_CLASS, ctx, \
      |  ^~~~~~~~~~~~~~~~~~~~
sof/sof/src/lib/dai.c:85:3: note: in expansion of macro 'tr_err'
   85 |   tr_err(&dai_tr, "dai_group_get(): invalid group_id %u",
      |   ^~~~~~

@iuliana-prodan iuliana-prodan mentioned this pull request May 20, 2021
Merged
@lgirdwood lgirdwood merged commit 8357dcf into thesofproject:main May 21, 2021
@marc-hb marc-hb requested a review from andyross May 21, 2021 18:32
@marc-hb
Copy link
Collaborator

marc-hb commented May 21, 2021

The #ifdef __ZEPHYR__ does not make sense to me because this entire file is used only when __ZEPHYR__ anyway. Can you please try to remove it?

The commit message does not make sense to me either, it does not seem to match the code change. Instead it should have explained why this is required with your toolchain but not needed in the configurations already built in CI.

Speaking of toolchains and CI, please add iMX to the list of SUPPORTED_PLATFORMS at the top of ./xtensa-build-zephyr.sh to add it to CI and make sure no regression like this happens again.

@iuliana-prodan
Copy link
Contributor Author

iuliana-prodan commented May 21, 2021

The #ifdef __ZEPHYR__ does not make sense to me because this entire file is used only when __ZEPHYR__ anyway. Can you please try to remove it?

You're right, I shouldn't add the #ifdef __ZEPHYR__. I will remove it.

The commit message does not make sense to me either, it does not seem to match the code change. Instead it should have explained why this is required with your toolchain but not needed in the configurations already built in CI.

I've added the warnings in a comment - see here.
I thought is too long to add it in the commit message.
Why we have the warnings with imx toolchain and not Intel, I don't know :(

Speaking of toolchains and CI, please add iMX to the list of SUPPORTED_PLATFORMS at the top of ./xtensa-build-zephyr.sh to add it to CI and make sure no regression like this happens again.

I'm planning of adding imx in ./xtensa-build-zephyr.sh (I've also answered this here), but maybe we should wait for the Zephyr pull request to be merged - see #35467.

@marc-hb
Copy link
Collaborator

marc-hb commented May 21, 2021

You're right, I shouldn't add the #ifdef ZEPHYR. I will remove it.

Thanks!

I thought is too long to add it in the commit message.

warning: implicit declaration of function 'printk' would have been enough (and accurate)

Why we have the warnings with imx toolchain and not Intel, I don't know :(

You can say "I don't know why" in the commit message and sometimes in the source code even. It happens :-)

"What gets us into trouble is not what we don't know. It's what we know for sure that just ain't so.”

but maybe we should wait for the Zephyr pull request to be merged

Agreed; let's not add to CI something that does not work yet. Otherwise people stop looking at it. Thanks for the links and reminders.

@marc-hb
Copy link
Collaborator

marc-hb commented May 21, 2021 •
edited
Loading

Here are some details about my configuration that does not need this commit for the record and in case anyone would like to get to the bottom of this.

Besides the pre-processor (and doxygen a bit), no one ever understands the maze of #includes and #defines in any C project. So the fastest way to extract #include information from the toolchain is to purposely break the code:

warning: passing argument 1 of 'printk' makes pointer from integer without a cast [-Wint-conversion]
  160 |  printk(42);
      |         ^~
      |         |
      |         int
In file included from zephyr/include/kernel_includes.h:32,
                 from zephyr/include/kernel.h:17,
                 from sof/zephyr/../src/include/sof/debug/panic.h:34,
                 from sof/zephyr/../src/include/sof/audio/audio_stream.h:18,
                 from sof/zephyr/../src/include/sof/audio/buffer.h:11,
                 from sof/src/trace/dma-trace.c:7:
zephyr/include/sys/printk.h:59:52: note: expected 'const char *' but argument is of type 'int'
   59 | extern __printf_like(1, 2) void printk(const char *fmt, ...);
      |                                        ~~~~~~~~~~~~^~~

$ ninja -j1 -C build-apl/ -v

ccache /$HOME/zephyr-sdk-0122/xtensa/intel_apl_adsp/xtensa-zephyr-elf/bin/xtensa-zephyr-elf-gcc 
-DBUILD_VERSION=v2.6.0-rc1-72-g469a10a0f9c9 -DCC_OPTIMIZE_FLAGS=\"-Os\" -DKERNEL 
-DSOF_GIT_TAG=\"\" -DSOF_MAJOR=3 -DSOF_MICRO=1 -DSOF_MINOR=2 -DSOF_SRC_HASH=0x0 -DSOF_TAG=\"7eff1\" 
-DXCC_TOOLS_VERSION=\"zephyr\" -D_FORTIFY_SOURCE=2 -D__ZEPHYR__=1 -DRELATIVE_FILE=\"../../../../build/SOF/sof/src/trace/dma-trace.c\" -I/___/zephyr/include -Izephyr/include/generated 
-I/___/zephyr/soc/xtensa/intel_adsp/cavs_v15 -I/___/zephyr/soc/xtensa/intel_adsp/cavs_v15/include 
-I/___/zephyr/soc/xtensa/intel_adsp/common/include -I/build/SOF/sof/zephyr/include -I/build/SOF/sof/zephyr/../src/platform/intel/cavs/include -I/build/SOF/sof/zephyr/../src/platform/apollolake/include 
-I/___/modules/hal/xtensa/include -I/___/modules/hal/xtensa/zephyr/soc/intel_apl_adsp 
-I/build/SOF/sof/zephyr/../rimage/src/include -I/build/SOF/sof/zephyr/../zephyr/include -I/build/SOF/sof/zephyr/../src/include 
-I/build/SOF/sof/zephyr/../src/arch/xtensa/include -isystem /___/zephyr/lib/libc/minimal/include -isystem /$HOME/zephyr-sdk-0122/xtensa/intel_apl_adsp/xtensa-zephyr-elf/bin/../lib/gcc/xtensa-zephyr-elf/10.2.0/include -isystem /$HOME/zephyr-sdk-0122/xtensa/intel_apl_adsp/xtensa-zephyr-elf/bin/../lib/gcc/xtensa-zephyr-elf/10.2.0/include-fixed 
-Os -imacros /___/build-apl/zephyr/include/generated/autoconf.h -ffreestanding -fno-common -g -fdiagnostics-color=always -imacros /___/zephyr/include/toolchain/zephyr_stdint.h
-Wall -Wformat -Wformat-security -Wno-format-zero-length -Wno-main -Wno-pointer-sign -Wpointer-arith -Wexpansion-to-defined -Wno-address-of-packed-member -Wno-unused-but-set-variable -Werror=implicit-int -fno-asynchronous-unwind-tables -fno-pie -fno-pic -fno-strict-overflow -fno-reorder-functions -fno-defer-pop -fmacro-prefix-map=/___/zephyr/samples/subsys/audio/sof=CMAKE_SOURCE_DIR -fmacro-prefix-map=/___/zephyr=ZEPHYR_BASE -fmacro-prefix-map=/home/mherber2/zephyr-west=WEST_TOPDIR -ffunction-sections -fdata-sections -mlongcalls 
-std=c99 -nostdinc -std=gnu99 -fno-inline-functions -MD -MT modules/sof/CMakeFiles/modules_sof.dir/build/SOF/sof/src/trace/dma-trace.c.obj -MF 
modules/sof/CMakeFiles/modules_sof.dir/build/SOF/sof/src/trace/dma-trace.c.obj.d -o 
modules/sof/CMakeFiles/modules_sof.dir/build/SOF/sof/src/trace/dma-trace.c.obj   -c /build/SOF/sof/src/trace/dma-trace.c
/build/SOF/sof/src/trace/dma-trace.c:160:9: error: 'breakage' undeclared (first use in this function)
  160 |  printk(breakage);
      |         ^~~~~~~~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dbaluta dbaluta dbaluta approved these changes

@lyakh lyakh lyakh approved these changes

@paulstelian97 paulstelian97 paulstelian97 approved these changes

@lgirdwood lgirdwood lgirdwood approved these changes

@akloniex akloniex Awaiting requested review from akloniex akloniex is a code owner

@ViorelSuman ViorelSuman Awaiting requested review from ViorelSuman

@kv2019i kv2019i Awaiting requested review from kv2019i

@andyross andyross Awaiting requested review from andyross

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@iuliana-prodan @marc-hb @dbaluta @lyakh @paulstelian97 @lgirdwood